Skip to content

[Dependency Scanning] Bridge Clang dependency scanner results on-demand #83600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Aug 7, 2025

#83050 changed the behavior to query every Clang module identifier imported from Swift, even for modules which may have previously been discovered as transitive dependencies of another query. This means we now have a lot of results coming in from Clang scanner queries which are redundant, and we bridge them all, all of the time, which adds up to a substantial amount of time.

Instead of always bridging all of the discovered modules of all of the queries, only do so for modules which have not already been cached at the time we record query results.

@artemcm
Copy link
Contributor Author

artemcm commented Aug 7, 2025

@swift-ci smoke test

@artemcm artemcm force-pushed the NoRedundantClangDepBridging branch from f345188 to 4876b2f Compare August 7, 2025 22:28
@artemcm
Copy link
Contributor Author

artemcm commented Aug 7, 2025

@swift-ci smoke test

@artemcm artemcm force-pushed the NoRedundantClangDepBridging branch from 4876b2f to 8cf2714 Compare August 12, 2025 16:10
@artemcm
Copy link
Contributor Author

artemcm commented Aug 12, 2025

@swift-ci smoke test

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function wise it is all good. Just some comments about the APIs.

clang::tooling::dependencies::ModuleOutputKind)>;

ModuleDependencyInfo
bridgeClangModuleDependency(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a static method as implementation details.

Or maybe it can be a private method in DependencyScanner? The callbacks all have a capture of the scanner reference, which is probably fine since scanner should outlive everything but if the bridging callbacks are part of the scanner, then there is no need for callback or worry about lifetimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This required a bigger refactor than it originally seemed, but I moved the bridging code into a private scanner utility.

@artemcm
Copy link
Contributor Author

artemcm commented Aug 13, 2025

@swift-ci smoke test

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Aug 13, 2025

@swift-ci smoke test

@artemcm
Copy link
Contributor Author

artemcm commented Aug 13, 2025

@swift-ci smoke test Linux platform

Copy link
Contributor

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly better. Still some comments about how functions are layered.

@@ -163,6 +164,12 @@ using ModuleDependencyIDSetVector =
llvm::SetVector<ModuleDependencyID, std::vector<ModuleDependencyID>,
std::set<ModuleDependencyID>>;

/// A callback to lookup module outputs for "-fmodule-file=", "-o" etc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are left over from previous change? Those are not used in this header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to ModuleDependencyScanner.h where the uses are.

/// \returns \c true if an error occurred, \c false otherwise
bool scanHeaderDependenciesOfSwiftModule(
const ASTContext &ctx,
/// of a Swift module.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the Doxygen comments are removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Instead of always bridging all of the discovered modules of all of the queries, only do so for modules which are not already cached
@artemcm artemcm force-pushed the NoRedundantClangDepBridging branch from 36ea30d to d228d30 Compare August 15, 2025 22:27
@artemcm
Copy link
Contributor Author

artemcm commented Aug 15, 2025

@swift-ci smoke test

…leDependencyScanner' utility

This moves the functionality of 'bridgeClangModuleDependency' into a utility in the main scanner class because it relies on various objects whose lifetime is already tied to the scanner itself.
@artemcm artemcm force-pushed the NoRedundantClangDepBridging branch from d228d30 to 9f0083c Compare August 15, 2025 22:40
@artemcm
Copy link
Contributor Author

artemcm commented Aug 15, 2025

@swift-ci smoke test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants